cli/command/registry: preserve all whitespace in secrets #6784

thaJeztah · 2026-02-07T12:18:59Z

fixes password with trailing space not accepted #6782
updates a21a5f4 / cli/command: PromptUserForCredentials: assorted minor improvements and (linting) fixes #5550

cli/command/registry: remove uses of "gotest.tools/v3/fs"

cli/command/registry: preserve all whitespace in secrets

Preserve all whitespace and treat the secret as an opaque value,
leaving it to the registry to (in)validate. We still check for
empty values in some places.

This partially reverts a21a5f4,
but checks for empty (whitespace-only) passwords without mutating
the value.

This better aligns with NIST SP 800-63B §5.1.1.2, which describes
that the value should be treated as opaque, preserving any other whitespace,
including newlines. Note that trimming whitespace may still happen elsewhere
(see NIST SP 800-63B (revision 4) §3.1.1.2);

Verifiers MAY make limited allowances for mistyping (e.g., removing
leading and trailing whitespace characters before verification, allowing
the verification of passwords with differing cases for the leading character)

- Human readable description for the release notes

Preserve leading and trailing whitespace when storing passwords.

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2026-02-07T12:20:34Z

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cli/command/registry/login.go	71.42%	2 Missing and 2 partials ⚠️
cli/command/registry.go	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

This PR updates docker login credential handling to preserve leading/trailing whitespace in secrets (treating the password as an opaque value), addressing #6782 and partially reverting behavior introduced in a21a5f4 / #5550.

Changes:

Add readSecretFromStdin() to trim only terminal line-endings (LF/CRLF/CR) when reading --password-stdin, without trimming other whitespace.
Stop mutating argPassword via strings.TrimSpace in PromptUserForCredentials, while still treating whitespace-only passwords as empty for prompting.
Update registry login tests: remove gotest.tools/v3/fs usage, use t.TempDir(), and add regression tests for passwords containing leading/trailing spaces and newline handling via stdin.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
cli/command/registry/login.go	Introduces `readSecretFromStdin()` and uses it for `--password-stdin` to preserve whitespace (except final line-ending).
cli/command/registry/login_test.go	Refactors temp file handling away from `fs`, injects stdin for password-stdin cases, and adds whitespace-related regression tests.
cli/command/registry.go	Avoids trimming `argPassword` while still detecting whitespace-only passwords as empty for prompting.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-07T13:38:36Z

cli/command/registry.go


-	argPassword = strings.TrimSpace(argPassword)
-	if argPassword == "" {
+	isEmpty := strings.TrimSpace(argPassword) == ""


This change avoids trimming argPassword, but passwords entered interactively are still trimmed because prompt.ReadInput() applies strings.TrimSpace(scanner.Text()) (internal/prompt/prompt.go:58). As a result, leading/trailing whitespace in passwords will still be lost when the password is prompted, which contradicts the PR’s goal of preserving whitespace. Consider adding a non-trimming prompt function (or an option/flag to ReadInput) for secrets so the returned password preserves whitespace (while still stripping the final line-ending).

Suggested change

isEmpty := strings.TrimSpace(argPassword) == ""

isEmpty := argPassword == ""

This is intentional; a whitespace-only password should not be considered valid, and so ignored here (to handle accidental user-input)

cli/command/registry/login_test.go

vvoland

This is probably a breaking change. Should we have an opt-out/opt-in for this?

vvoland · 2026-02-09T08:35:43Z

cli/command/registry/login.go

+	n := len(b)
+	switch b[n-1] {
+	case '\n':
+		if n >= 2 && b[n-2] == '\r' {
+			b = b[:n-2]
+		} else {
+			b = b[:n-1]
+		}
+	case '\r':
+		b = b[:n-1]
+	}


nit, something like this would be a bit more readable IMHO:

Suggested change

n := len(b)

switch b[n-1] {

case '\n':

if n >= 2 && b[n-2] == '\r' {

b = b[:n-2]

} else {

b = b[:n-1]

}

case '\r':

b = b[:n-1]

}

switch {

case bytes.HasSuffix(b, []byte("\r\n")):

b = b[:len(b)-2]

case bytes.HasSuffix(b, []byte("\n")), bytes.HasSuffix(b, []byte("\r")):

b = b[:len(b)-1]

}

or

Suggested change

n := len(b)

switch b[n-1] {

case '\n':

if n >= 2 && b[n-2] == '\r' {

b = b[:n-2]

} else {

b = b[:n-1]

}

case '\r':

b = b[:n-1]

}

for _, eol := range []string{"\r\n", "\n", "\r"} {

if bytes.HasSuffix(b, []byte(eol)) {

return string(b[:len(b)-len(eol)]), nil

}

}

Ah, yeah, I was on the fence; originally had this (using CutSuffix which doesn't allocate, and returns an ok); the []byte was a bit ugly, but perhaps still fine;

for _, eol := range [][]byte{[]byte("\r\n"), []byte("\n"), []byte("\r")} { var ok bool b, ok = bytes.CutSuffix(b, eol) if ok { break } }

I can change to that

Yeah this is much cleaner!

Updated 👍

thaJeztah · 2026-02-09T10:33:42Z

This is probably a breaking change. Should we have an opt-out/opt-in for this?

Tricky one; so we already preserved whitespace elsewhere, and before a21a5f4 we didn't trim. It kinda makes sense to store things as-is; in general we'd already have to account for stored credentials to be stored through other means, so we can't expect them to be trimmed (or otherwise).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Preserve all whitespace and treat the secret as an opaque value, leaving it to the registry to (in)validate. We still check for empty values in some places. This partially reverts a21a5f4, but checks for empty (whitespace-only) passwords without mutating the value. This better aligns with [NIST SP 800-63B §5.1.1.2], which describes that the value should be treated as opaque, preserving any other whitespace, including newlines. Note that trimming whitespace may still happen elsewhere (see [NIST SP 800-63B (revision 4) §3.1.1.2]); > Verifiers **MAY** make limited allowances for mistyping (e.g., removing > leading and trailing whitespace characters before verification, allowing > the verification of passwords with differing cases for the leading character) [NIST SP 800-63B §5.1.1.2]: https://pages.nist.gov/800-63-3/sp800-63b.html#memsecretver [NIST SP 800-63B (revision 4) §3.1.1.2]: https://pages.nist.gov/800-63-4/sp800-63b.html#passwordver Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vvoland

LGTM, can this go into a patch release or should we have a minor for that?

thaJeztah · 2026-02-09T13:51:35Z

I THINK either way is probably fine; as mentioned, before a21a5f4, we didn't trim either. Overall it would be a pretty corner-case probably.

@Benehiko thoughts?

Benehiko

LGTM

I guess if we release in a patch it could be strange if logins suddenly failed due to a buggy script adding a space that was never discovered 😆
But I'm sure a release in a patch should be alright.

thaJeztah added this to the 29.2.2 milestone Feb 7, 2026

thaJeztah added impact/changelog kind/enhancement status/2-code-review kind/bugfix PR's that fix bugs area/authentication labels Feb 7, 2026

thaJeztah force-pushed the login_cleanups branch 2 times, most recently from 7742eae to ed7ad1a Compare February 7, 2026 12:23

thaJeztah requested review from Benehiko, Copilot and vvoland February 7, 2026 13:32

Copilot started reviewing on behalf of thaJeztah February 7, 2026 13:33 View session

Copilot AI reviewed Feb 7, 2026

View reviewed changes

thaJeztah force-pushed the login_cleanups branch from ed7ad1a to 378bbc2 Compare February 7, 2026 14:11

vvoland reviewed Feb 9, 2026

View reviewed changes

thaJeztah added 2 commits February 9, 2026 13:55

cli/command/registry: remove uses of "gotest.tools/v3/fs"

c435940

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the login_cleanups branch from 378bbc2 to ac38140 Compare February 9, 2026 12:55

vvoland approved these changes Feb 9, 2026

View reviewed changes

Benehiko approved these changes Feb 9, 2026

View reviewed changes

	isEmpty := strings.TrimSpace(argPassword) == ""
	isEmpty := argPassword == ""

cli/command/registry: preserve all whitespace in secrets #6784

Are you sure you want to change the base?

cli/command/registry: preserve all whitespace in secrets #6784

Conversation

thaJeztah commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cli/command/registry: remove uses of "gotest.tools/v3/fs"